Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624

Kielek · 2024-03-25T12:55:27Z

Fixes: issue when it is hard to determine what is the instrumentation library version used in end-user project.
Most of packages just reporte 1.0.0.0 (Assembly version).
MinVer is setting more user friendly value in AssemblyInformationalVersionAttribute attribute in following format: 1.7.2-alpha.0.39+69cc79aed00a37c103b9e46488e2001db959a1ab.

It was previously set correctly only in one package.

Changes

Instrumentation spans - set ActivitySource.Version and Meter.Version to NuGet package version

For significant contributions please make sure you have completed the following items:

Appropriate CHANGELOG.md updated for non-trivial changes
~~[ ] Design discussion issue #~~
~~[ ] Changes in public API reviewed~~

…ersion

codecov · 2024-03-25T12:57:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.82%. Comparing base (71655ce) to head (8d9a225).
Report is 175 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    open-telemetry/opentelemetry-dotnet#1624      +/-   ##
==========================================
- Coverage   73.91%   73.82%   -0.10%     
==========================================
  Files         267      263       -4     
  Lines        9615     9783     +168     
==========================================
+ Hits         7107     7222     +115     
- Misses       2508     2561      +53

Flag	Coverage Δ
unittests-Exporter.Geneva	`59.81% <ø> (?)`
unittests-Exporter.OneCollector	`89.72% <ø> (?)`
unittests-Extensions	`82.75% <ø> (?)`
unittests-Instrumentation.AspNet	`77.55% <100.00%> (?)`
unittests-Instrumentation.EventCounters	`75.92% <100.00%> (?)`
unittests-Instrumentation.Owin	`85.61% <100.00%> (?)`
unittests-Instrumentation.Process	`100.00% <100.00%> (?)`
unittests-Instrumentation.Runtime	`100.00% <100.00%> (?)`
unittests-Instrumentation.StackExchangeRedis	`70.89% <100.00%> (?)`
unittests-Instrumentation.Wcf	`78.44% <100.00%> (?)`
unittests-PersistentStorage	`65.78% <ø> (?)`
unittests-ResourceDetectors.Azure	`81.53% <ø> (?)`
unittests-ResourceDetectors.Host	`40.00% <ø> (?)`
unittests-ResourceDetectors.Process	`83.21% <ø> (?)`
unittests-ResourceDetectors.ProcessRuntime	`76.08% <ø> (?)`
unittests-Solution	`79.43% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...on.AWS/Implementation/AWSTracingPipelineHandler.cs	`85.41% <100.00%> (-2.34%)`	⬇️
...etry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs	`94.54% <100.00%> (-0.29%)`	⬇️
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs	`85.71% <100.00%> (+0.46%)`	⬆️
...nTelemetry.Instrumentation.AspNet/AspNetMetrics.cs	`100.00% <100.00%> (ø)`
.../ElasticsearchRequestPipelineDiagnosticListener.cs	`82.20% <100.00%> (ø)`
...mplementation/EntityFrameworkDiagnosticListener.cs	`52.89% <100.00%> (+0.51%)`	⬆️
...trumentation.EventCounters/EventCountersMetrics.cs	`81.17% <100.00%> (+1.17%)`	⬆️
...Hangfire/Implementation/HangfireInstrumentation.cs	`100.00% <100.00%> (ø)`
...mplementation/OwinInstrumentationActivitySource.cs	`100.00% <100.00%> (ø)`
....Owin/Implementation/OwinInstrumentationMetrics.cs	`100.00% <100.00%> (ø)`
... and 5 more

... and 190 files with indirect coverage changes

Kielek · 2024-03-25T17:59:50Z

I will be happy to propagate to main repository if we have agree on the naming and values.

utpilla · 2024-03-25T21:40:23Z

Why does the ActivitySource version need to match the package version?

utpilla · 2024-03-25T21:45:46Z

I will be happy to propagate to main repository if we have agree on the naming and values.

This would probably be considered a breaking change for the stable packages.

cijothomas · 2024-03-25T22:35:39Z

reporting nuget version looks better than assembly version which is 1.0.0.0 always, but i wonder if there was any guidance on this in terms of using nuget version.. Spec says library version as an example, so it is probably good to use nuget version.

I will be happy to propagate to main repository if we have agree on the naming and values.

This would probably be considered a breaking change for the stable packages.

I am not sure how would this be a breaking change? It is not changing the semantics on the telemetry itself. Also there is no way to enable/disable based on Version today in the SDK. Ofcourse it should be possible in collector possibly, but still this does not look like breaking change.

vishweshbankwar · 2024-03-25T23:13:50Z

Just the heads up. This will work well for libraries that create their own ActivitySource. It won't work for libraries that are simply enriching the activity created by respective frameworks. e.g. OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.HttpClient

So there will still be some inconsistency/gaps.

utpilla · 2024-03-25T23:53:06Z

I am not sure how would this be a breaking change? It is not changing the semantics on the telemetry itself. Also there is no way to enable/disable based on Version today in the SDK. Ofcourse it should be possible in collector possibly, but still this does not look like breaking change.

I was thinking about its implications for someone using a custom ActivityListener but I guess we don't need to worry about that since the ActvitySource instances are not marked public for both the stable instrumentation packages.

What problem are we trying to solve though? Who is using ActivitySource version and for what purpose? Also, as @vishweshbankwar pointed out we do not have any control over the libraries that natively support ActivitySource based instrumentation, so we wouldn't have any kind of consistency either.

cijothomas · 2024-03-26T00:13:14Z

I am not sure how would this be a breaking change? It is not changing the semantics on the telemetry itself. Also there is no way to enable/disable based on Version today in the SDK. Ofcourse it should be possible in collector possibly, but still this does not look like breaking change.

I was thinking about its implications for someone using a custom ActivityListener but I guess we don't need to worry about that since the ActvitySource instances are not marked public for both the stable instrumentation packages.

Got it!

cijothomas · 2024-03-26T00:15:16Z

Just the heads up. This will work well for libraries that create their own ActivitySource. It won't work for libraries that are simply enriching the activity created by respective frameworks. e.g. OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.HttpClient

So there will still be some inconsistency/gaps.

Good point. Same challenges for "schema_url" too (not yet supported in .NET)!

Kielek · 2024-03-26T05:35:45Z

Just the heads up. This will work well for libraries that create their own ActivitySource. It won't work for libraries that are simply enriching the activity created by respective frameworks. e.g. OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.HttpClient

So there will still be some inconsistency/gaps.

I would not say that inconsistency. As a follow up to this (and main reposiroty) we should start recommending what to put as a version. For most cases it should be not assembly version, as it typically is nonzero.0.0.0. Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

Oberon00 · 2024-03-26T07:18:13Z

For most cases it should be not assembly version, as it typically is nonzero.0.0.0

Shouldn't we fix the assembly version then?

Kielek · 2024-03-26T07:44:09Z

For most cases it should be not assembly version, as it typically is nonzero.0.0.0

Shouldn't we fix the assembly version then?

It is against this recommendation: https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/versioning#assembly-version

Oberon00 · 2024-03-26T07:59:32Z

src/OpenTelemetry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs

@@ -21,8 +20,7 @@ public static class AWSLambdaWrapper
 {
    internal const string ActivitySourceName = "OpenTelemetry.Instrumentation.AWSLambda";

-    private static readonly string Version = typeof(AWSLambdaWrapper).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()!.InformationalVersion.Split('+')[0];
-    private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, Version);
+    private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaInstrumentationOptions>());


Any particular reason for changing the type from which we query the assembly? It should not make a difference, but the minimal change would have been:

Suggested change

private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaInstrumentationOptions>());

private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaWrapper>());

I have started with this :)

`OpenTelemetry.Instrumentation.AWSLambda.AWSLambdaWrapper': static class type cannot be used as type arguments

Oberon00 · 2024-03-26T08:00:26Z

src/Shared/SignalVersionHelper.cs

+{
+    public static string GetVersion<T>()
+    {
+        return typeof(T).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()!.InformationalVersion.Split('+')[0];


Since we are using !. here, would it be better to use any fallback, or maybe a ?. chain?

I do not see any case when the null value can happen in any of library.
As long as we are using MinVer'sioning it should be safe to use this construction.

cijothomas · 2024-03-26T15:54:42Z

@Kielek Would it be possible for you to take this to the OTel community meeting? If you cannot make it to the meeting, @vishweshbankwar can you help?

utpilla · 2024-03-26T17:52:59Z

As a follow up to this (and main reposiroty) we should start recommending what to put as a version.

Users are not required to provide a version. The spec already says that version is optional. I don't think there is any need for us to recommend what value to use for it.

Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

How are users using ActivitySource version for debugging purposes?

Kielek · 2024-03-26T18:22:41Z

Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

How are users using ActivitySource version for debugging purposes?

From observability vendors perspective, if this value is associated with spans, we can easily check what version of instrumentation is used. It is valid especially if customers are using library instrumentations. Based in such data, we can recommend some steps, such us upgrade if fixes are in newer versions.

ppittle · 2024-03-27T17:54:17Z

src/Shared/SignalVersionHelper.cs

+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+using System.Reflection;


naïve question: Does this have any implication for AOT? Does AOT support getting assembly info?

I do not see any compilation warnings. Some instrumentation packages are included into AOT app.

opentelemetry-dotnet-contrib/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj

Lines 18 to 22 in e39306f

<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWS" />

<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWSLambda" />

<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.EventCounters" />

<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.Runtime" />

<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.StackExchangeRedis" />

cijothomas · 2024-03-27T20:10:53Z

Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

How are users using ActivitySource version for debugging purposes?

From observability vendors perspective, if this value is associated with spans, we can easily check what version of instrumentation is used. It is valid especially if customers are using library instrumentations. Based in such data, we can recommend some steps, such us upgrade if fixes are in newer versions.

Got it!
Another example: a vendor has some special handling for v1 of semantic conventions, and want to get rid of that special handling, as v2 is out now, and v2 is considered stable. But the vendor need to figure out what percentage of its users is producing v1 vs v2.
This can be understood by looking at instrumentationscope.version.

Agree that assembly version won't be able to give such breakdowns.! (the special set of instrumentations where scope is owned by one component, but actual filling of data is done by another won't work as already pointed out, but that is also a temporary state, assuming these libraries will eventually natively produce telemetry without the need of a instrumentation library, like already done for metrics in some cases.)

Oberon00 · 2024-03-28T06:28:48Z

@cijothomas

But the vendor need to figure out what percentage of its users is producing v1 vs v2.
This can be understood by looking at instrumentationscope.version.

Not really. The same library may produce different versions, e.g. for HTTP depending on settings. Also, this use case is perfectly suited for the schema URL that is reported with each scope.

vishweshbankwar

LGTM: There is still open question on how to handle the version when instrumentations do not create the ActivitySource for the activities they enrich. None of the libraries here falls under that category.

@Kielek - We could open issues for the special cases in their respective repos.

Kielek · 2024-04-02T05:06:10Z

@vishweshbankwar, I created: #5497. Will merge this one shortly (when the CI will finished).

Instrumentation spans - set ActivitySource.Version to NuGet package v…

9d38859

…ersion

Kielek had a problem deploying to external March 25, 2024 12:55 — with GitHub Actions Failure

Kielek marked this pull request as ready for review March 25, 2024 13:11

Kielek requested a review from a team March 25, 2024 13:11

Merge branch 'main' into activity-source-version

6ab5896

Kielek had a problem deploying to external March 25, 2024 18:00 — with GitHub Actions Failure

github-actions bot assigned CodeBlanch, ejsmith, fred2u, maldago, Oberon00, pcwiese, ppittle, rypdal and srprash Mar 25, 2024

Merge branch 'main' into activity-source-version

ee1bbcd

Kielek had a problem deploying to external March 25, 2024 21:01 — with GitHub Actions Failure

Merge branch 'main' into activity-source-version

cd21c2b

Kielek had a problem deploying to external March 26, 2024 05:35 — with GitHub Actions Failure

Instrumentation metrics - set Meter.Version to NuGet package version

99dff8c

Oberon00 approved these changes Mar 26, 2024

View reviewed changes

fred2u approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'main' into activity-source-version

ecf852e

Kielek had a problem deploying to external March 27, 2024 15:56 — with GitHub Actions Failure

ppittle reviewed Mar 27, 2024

View reviewed changes

Merge branch 'main' into activity-source-version

ada8a61

Kielek had a problem deploying to external March 27, 2024 19:07 — with GitHub Actions Failure

Merge branch 'main' into activity-source-version

3f50d55

Kielek had a problem deploying to external March 29, 2024 08:05 — with GitHub Actions Failure

Merge branch 'main' into activity-source-version

4620766

Kielek had a problem deploying to external March 29, 2024 20:01 — with GitHub Actions Failure

vishweshbankwar approved these changes Mar 29, 2024

View reviewed changes

ppittle approved these changes Mar 29, 2024

View reviewed changes

Merge branch 'main' into activity-source-version

8d9a225

Kielek had a problem deploying to external April 2, 2024 05:04 — with GitHub Actions Failure

Kielek mentioned this pull request Apr 2, 2024

[Instrumentations] ActivitySource and Meter versioning #1758

Open

Kielek merged commit 11f5c2c into open-telemetry:main Apr 2, 2024
112 of 113 checks passed

Kielek deleted the activity-source-version branch April 2, 2024 05:12

Kielek mentioned this pull request Apr 16, 2024

README update: Add Oracle .NET OpenTelemetry provider open-telemetry/opentelemetry-dotnet-instrumentation#3192

Closed

Kielek mentioned this pull request Aug 28, 2024

[Instrumentation.StackExchangeRedis] Metrics support #1982

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624

Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624

Kielek commented Mar 25, 2024 •

edited

Loading

codecov bot commented Mar 25, 2024 •

edited

Loading

Kielek commented Mar 25, 2024

utpilla commented Mar 25, 2024

utpilla commented Mar 25, 2024

cijothomas commented Mar 25, 2024

vishweshbankwar commented Mar 25, 2024 •

edited

Loading

utpilla commented Mar 25, 2024

cijothomas commented Mar 26, 2024

cijothomas commented Mar 26, 2024

Kielek commented Mar 26, 2024

Oberon00 commented Mar 26, 2024

Kielek commented Mar 26, 2024

Oberon00 Mar 26, 2024

Kielek Mar 26, 2024

Oberon00 Mar 26, 2024

Kielek Mar 26, 2024

cijothomas commented Mar 26, 2024

utpilla commented Mar 26, 2024

Kielek commented Mar 26, 2024

ppittle Mar 27, 2024

Kielek Mar 27, 2024

cijothomas commented Mar 27, 2024

Oberon00 commented Mar 28, 2024 •

edited

Loading

vishweshbankwar left a comment

Kielek commented Apr 2, 2024 •

edited

Loading

	private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaInstrumentationOptions>());
	private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaWrapper>());

	<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWS" />
	<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWSLambda" />
	<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.EventCounters" />
	<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.Runtime" />
	<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.StackExchangeRedis" />

Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624

Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624

Conversation

Kielek commented Mar 25, 2024 • edited Loading

Changes

codecov bot commented Mar 25, 2024 • edited Loading

Codecov Report

Kielek commented Mar 25, 2024

utpilla commented Mar 25, 2024

utpilla commented Mar 25, 2024

cijothomas commented Mar 25, 2024

vishweshbankwar commented Mar 25, 2024 • edited Loading

utpilla commented Mar 25, 2024

cijothomas commented Mar 26, 2024

cijothomas commented Mar 26, 2024

Kielek commented Mar 26, 2024

Oberon00 commented Mar 26, 2024

Kielek commented Mar 26, 2024

Oberon00 Mar 26, 2024

Choose a reason for hiding this comment

Kielek Mar 26, 2024

Choose a reason for hiding this comment

Oberon00 Mar 26, 2024

Choose a reason for hiding this comment

Kielek Mar 26, 2024

Choose a reason for hiding this comment

cijothomas commented Mar 26, 2024

utpilla commented Mar 26, 2024

Kielek commented Mar 26, 2024

ppittle Mar 27, 2024

Choose a reason for hiding this comment

Kielek Mar 27, 2024

Choose a reason for hiding this comment

cijothomas commented Mar 27, 2024

Oberon00 commented Mar 28, 2024 • edited Loading

vishweshbankwar left a comment

Choose a reason for hiding this comment

Kielek commented Apr 2, 2024 • edited Loading

Kielek commented Mar 25, 2024 •

edited

Loading

codecov bot commented Mar 25, 2024 •

edited

Loading

vishweshbankwar commented Mar 25, 2024 •

edited

Loading

Oberon00 commented Mar 28, 2024 •

edited

Loading

Kielek commented Apr 2, 2024 •

edited

Loading